feat: add OneKey keyring #353
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
2dc83be to
a18c779
Compare
Akaryatrh
added
the
team-hardware-wallets
|
@metamaskbot publish-preview |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
2 similar comments
|
@metamaskbot publish-preview |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
ca02a28 to
89518ee
Compare
|
@SocketSecurity ignore npm/async-function |
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
89518ee to
ca5bb42
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
| reject(new Error('signature doesnt match the right address')); | ||
| } | ||
| // eslint-disable-next-line promise/no-multiple-resolved | ||
| resolve(signature); |
There was a problem hiding this comment.
Missing return after reject causes double promise resolution
Medium Severity
In signPersonalMessage, when the recovered signature address doesn't match the expected account, reject() is called but execution continues and resolve(signature) is also invoked. The eslint-disable-next-line promise/no-multiple-resolved comment confirms this was caught by the linter but suppressed rather than fixed. While Promise semantics mean the rejection wins, calling both reject and resolve on the same promise is incorrect behavior and could lead to confusion or bugs if the code is modified later. A return statement is needed after the reject() call.
| #onUIEvent?: ((event: Unsuccessful['payload']) => void) | undefined; | ||
|
|
||
| #handleBlockErrorEvent(payload: Unsuccessful): void { | ||
| const { code } = payload.payload; |
There was a problem hiding this comment.
Missing null check causes crash in error handler
Medium Severity
The #handleBlockErrorEvent method immediately destructures payload.payload without a null check, but callers pass result after checking result?.success with optional chaining. This inconsistency means if an SDK method returns null or undefined, the optional chaining check passes (since undefined?.success is falsy), but then #handleBlockErrorEvent(result) crashes with a TypeError when accessing undefined.payload. The pattern repeats in getPublicKey, getPassphraseState, ethereumSignTransaction, ethereumSignMessage, and ethereumSignTypedData.
Additional Locations (2)
|
@SocketSecurity ignore npm/async-function@1.0.0 |
| reject(new Error('signature doesnt match the right address')); | ||
| } | ||
| // eslint-disable-next-line promise/no-multiple-resolved | ||
| resolve(signature); |
There was a problem hiding this comment.
Missing return after reject causes multiple promise settlements
Medium Severity
In signPersonalMessage, when the recovered address doesn't match the expected address, reject() is called but execution continues and resolve(signature) is also called. The eslint-disable comment for promise/no-multiple-resolved acknowledges this issue but masks it rather than fixing it. A return statement is needed after the reject() call to prevent the promise from being settled twice and to avoid returning an invalid signature path.
| }, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
UI_EVENT subscription never removed causes memory leak
Medium Severity
The UI_EVENT event listener added via this.sdk?.on('UI_EVENT', ...) in init() is never unsubscribed. Neither destroy() nor dispose() removes this listener before clearing the SDK reference. The callback closure holds references to this (the bridge instance), causing memory to accumulate if the bridge is initialized and destroyed multiple times during the application lifecycle.
Additional Locations (1)
| async dispose(): Promise<void> { | ||
| this.sdk?.dispose(); | ||
| return Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
dispose() doesn't reset state preventing SDK re-initialization
Medium Severity
The dispose() method calls sdk.dispose() but doesn't reset isSDKInitialized or sdk. Since OneKeyKeyring.destroy() calls bridge.dispose() (not bridge.destroy()), the bridge is left in an inconsistent state where isSDKInitialized remains true but the SDK is disposed. Subsequent calls to init() return early without re-initializing, and SDK method calls will fail on the disposed SDK.
Additional Locations (1)
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
e40d1ad to
15bb912
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
15bb912 to
acd41b7
Compare
|
@metamaskbot publish-preview |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
1 similar comment
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
|
||
| return this.hdPath === newHdPath; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing lock() method causes test runtime failure
High Severity
The OneKeyKeyring class is missing a lock() method, but the test file explicitly calls keyring.lock() and expects isUnlocked() to return false afterward. Since isUnlocked() returns Boolean(this.hdk?.publicKey), a lock() method that resets the HDKey is needed for the feature to work and for the test to pass. Without this method, the test will fail with a runtime error and the keyring cannot be locked programmatically.
Additional Locations (1)
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
feb98c5 to
2d9d5c0
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
Adds OneKey keyring based on OneKey libraries.
Note
Adds OneKey hardware wallet support to the monorepo via a new keyring package.
@metamask/eth-onekey-keyringpackage withOneKeyKeyringandOneKeyWebBridgeusing@onekeyfe/hd-web-sdkto handle PIN/passphrase UI, passphrase state, and transport switchingWritten by Cursor Bugbot for commit 2d9d5c0. This will update automatically on new commits. Configure here.